Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Detections] Adds Nested CTI row renderer #96275

Merged
merged 33 commits into from
Apr 16, 2021

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Apr 6, 2021

Summary

Adds a new row renderer for alerts enriched by threat indicator data.

  • User can enable a CTI Row Renderer that describes the circumstances of the indicator match
  • User can pivot to various views by sending details to the timeline, e.g.:
    • threat.indicator.matched.id to pivot on a specific indicator
    • threat.indicator.matched.atomic to pivot on the matched value
  • CTI Row Renderer is only applied to CTI alerts
  • Multiple inner "rows" are displayed for each indicator that matched

Row renderer on a single line:

Detections_-_Kibana

Row renderer on multiple lines:

Detections_-_Kibana

Row renderer with multiple matches:

Detections_-_Kibana

Row renderer example in Customize popover:

Detections_-_Kibana

TODO

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Copy link
Contributor

@ecezalp ecezalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great, fantastic work! I added a few comments that you may find useful. Two notes about the UI - when there are multiple rows, and a draggable has the same value as another (such as type: file), if you start dragging one of them, all of them disappear - I think what you need is unique keys. Also, attaching a screenshot of my alerts table on the Detections page - I definitely have some unusual "data" - nevertheless we could consider using a single "line" under circumstances that there is a lot of horizontal real estate available possibly with flex doing the positioning. And maybe some padding could be added.
Screen Shot 2021-04-07 at 5 00 27 PM

*/

interface ThreatMatchEcs {
atomic?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these truly optional? if you have an atomic, shouldn't you always have a field and a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing *Ecs types seemed to be very defensive, where almost everything was optional. If it's one of our detection alerts then it should have all three, but I think these are meant to be general to anything "compliant" with ECS.

import { ROW_RENDERER_BROWSER_EXAMPLE_TIMELINE_ID } from '../constants';

const ThreatMatchExampleComponent: React.FC = () => (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depending on the return value of the renderRow method, fragment can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, this was just copy/pasted from the other examples.

});

describe('#renderRow', () => {
it('renders correctly against snapshot', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to cover a scenario with multiple rows for a single alert

rylnd added 18 commits April 12, 2021 20:30
The display details are not yet implemented, but those will be fleshed
out in the ThreatMatchRow component.
This data is not used by any existing row renderers and so this commit
is mostly just plumbing that data through.

This is necessary, however, for our new threat match row renderer as it
requires nested fields, which cannot be retrieved through the mechanism
that retrieves the existing row renderer data. However, these nested
fields are available, if requested, through this other data structure,
hence this plumbing.

For now to minimize changes I'm marking this as an optional field;
however in reality a value will always be present.
Updates logic, tests and mocks accordingly.
* helpers
* explicit fields file, which will hopefully be part of the renderer API
  at some point
* parent component to split data into "rows" as defined by our renderer
* row component for stateless presentation of a single match
Adds tentative copy, example row, and accompanying mock data.
I haven't yet deleted the old (new?) unused path yet. Cleanup to come.
* Rewrites isInstance logic for new data as helper, hasThreatMatchValue
* Updating types and tests
  * Adds to the previously empty ThreatEcs
This reverts commit 19c93ee.

We ended up extending the existing data (albeit from the fields
response!).
* adds contextId and eventId to pass to draggable
* We don't have a order-independent key for each individual
  ThreatMatchRow, due to matched.id not being mapped/returned in the
  fields response
* Fixes up a few things related to using the new data format
* Adds missing Threat ECS types
In order to use these in both the row renderer and the server request,
we need to move them to common/
These are currently hardcoded on the backend of the events/all query
(via TIMELINE_EVENTS_FIELDS); declaring them on both ends is arguably
confusing, and we're going with YAGNI for now.
This was causing type errors as this enum exists both here and in
common/, and I had only updated one of them.
One is still failing due to an outdated test subject, but I expect this
to change after an upcoming meeting so leaving it for now.
One for displaying match details, and another for indicator details

The indicator details will be sparse, so there's going to be some
conditional rendering in there.
* Adds translations for copy
* Fixes most of our layout woes with more flexbox!
* Conditional rendering of indicator details based on data
* tests
These tests are all implicitly testing the list of row renderers.
At certain container widths, a half-width hr is not sufficient.
@rylnd rylnd marked this pull request as ready for review April 15, 2021 03:08
@rylnd rylnd requested a review from a team as a code owner April 15, 2021 03:08
@rylnd rylnd added the Team:Threat Hunting Security Solution Threat Hunting Team label Apr 15, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

Copy link
Contributor

@ecezalp ecezalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good. There is only one thing I noticed about the drag & drop functionality, it seemed like the entire row is disappearing while dragging a single field from the row, instead of just the field itself disappearing. That being said I experienced this with some exceptionally suboptimal data (two rows of identical content), so please take that with a grain of salt.

x-pack/plugins/security_solution/common/cti/constants.ts Outdated Show resolved Hide resolved
const getIndicatorEcs = (data: Ecs): ThreatIndicatorEcs[] =>
get(data, INDICATOR_DESTINATION_PATH) ?? [];

export const hasThreatMatchValue = (data: Ecs): boolean =>
Copy link
Contributor

@ecezalp ecezalp Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same logic in use_threat_intel_tabs.ts file, there is a bool called isThreatPresent that is determined in a similar way. we should consider consolidating those, ideally somewhere proximate to CTI constants, unless those were in the timeline directory - I guess that is so, where should it live?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks similar but definitely not identical. I agree that they should at least live close to each other or build from the same base functions; we'll have to tackle that when these are both merged.

interface IndicatorDetailsProps {
contextId: string;
eventId: string;
indicatorDataset: string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indicatorDataset?: string would be shorter

Copy link
Contributor Author

@rylnd rylnd Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not identical! My preference is for this form, as it prevents consumers from accidentally dropping a known value.

<>
<EuiFlexItem grow={false}>
<HorizontalSpacer>
<FormattedMessage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks a bit different from the i18n.FOO pattern that was present in other places for internationalized strings - is there an additional benefit to using the FormattedMessage component? would be happy to know

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a react wrapper around the same logic. Since I don't expect/intend for these strings to be shared, they can just live in the component itself (for now).

)}
{indicatorReference && (
<>
<EuiFlexItem grow={false}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think there could be a DRYer way around these <EuiFlexItem grow={false}> wrappers? maybe something super tiny such as IndicatorDetail = ({...props, children}) => <EuiFlexItem grow=false {...props}>{children}</EuiFlexItem>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then it would look like:

Suggested change
<EuiFlexItem grow={false}>
<IndicatorDetail>

?

There's not a huge benefit to that IMO, but we certainly could. I'm inclined to leave these dumb for now, at least until people start 👀 ing this row renderer and give UI feedback; premature DRYing often leads to unnecessary work.

justifyContent="center"
>
<EuiFlexItem grow={false}>
<MatchDetails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is interesting - I would have written this either without desctructuring the props, or without the MatchDetails / IndicatorDetails components (embedding the contents of those components directly into the ThreatMatchRowView instead). This looks nice and clean, but it is also slightly more verbose, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't disagree with anything you said :)

@@ -239,5 +232,8 @@ export const TIMELINE_EVENTS_FIELDS = [
'zeek.ssl.established',
'zeek.ssl.resumed',
'zeek.ssl.version',
...TIMELINE_CTI_FIELDS,
// TODO in the future, our alerts timeline fields should be derived from the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a ticket for this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a ticket for this work?

API changes for RAC: #94520

@andrew-goldstein
Copy link
Contributor

Thanks for pairing to desk test this @rylnd!

  • The following entry:
threat_match = 'threat_match',

is required in the RowRendererId enum in x-pack/plugins/security_solution/public/graphql/types.ts

After adding the above, run start-bean-gen to update the graphql types. (This resolves the error that appears in the TImelines tab, or when accessing the Open Timelines modal.)

if you start dragging one of them, all of them disappear - I think what you need is unique keys

This occurs when the same draggable fields are rendered twice per-row for a nested row renderer, because the context + field id + event id + value is no longer unique. Consider including the "nesting level" in the key to make the IDs unique again.

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new row renderer @rylnd!
Looks ready apart from the two items that came up while desk testing locally
LGTM 🚀

Obviates the need for the accompanying comments.
Also ensures less traffic to urlhaus ;)
We need to add the row index to our contextId to ensure that our
draggables work correctly for multiple rows, since each row will
necessarily have the same eventId and timelineId.
@rylnd
Copy link
Contributor Author

rylnd commented Apr 15, 2021

@elasticmachine merge upstream

@rylnd rylnd added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 15, 2021
@rylnd rylnd enabled auto-merge (squash) April 15, 2021 22:41
@rylnd
Copy link
Contributor Author

rylnd commented Apr 15, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2208 2216 +8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.3MB 7.3MB +12.2KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @rylnd

@rylnd rylnd merged commit 540924b into elastic:master Apr 16, 2021
@rylnd rylnd deleted the nested_row_renderer branch April 16, 2021 01:28
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 16, 2021
…#96275)

* Move alert-specific mocks to more declarative mock file

* Add placeholder interface for ECS threat fields

* Test and implement CTI row renderer

The display details are not yet implemented, but those will be fleshed
out in the ThreatMatchRow component.

* Pass full fields data to our row renderers

This data is not used by any existing row renderers and so this commit
is mostly just plumbing that data through.

This is necessary, however, for our new threat match row renderer as it
requires nested fields, which cannot be retrieved through the mechanism
that retrieves the existing row renderer data. However, these nested
fields are available, if requested, through this other data structure,
hence this plumbing.

For now to minimize changes I'm marking this as an optional field;
however in reality a value will always be present.

* Rewrite existing row renderer in terms of flattened data

Updates logic, tests and mocks accordingly.

* Moving logic into discrete files

* helpers
* explicit fields file, which will hopefully be part of the renderer API
  at some point
* parent component to split data into "rows" as defined by our renderer
* row component for stateless presentation of a single match

* Register threat match row rendere

Adds tentative copy, example row, and accompanying mock data.

* WIP: Rendering draggable fields but hit the data loss issue with nested fields being flattened

* WIP: implementing row renderer against new data format

I haven't yet deleted the old (new?) unused path yet. Cleanup to come.

* Updating based on new data

* Rewrites isInstance logic for new data as helper, hasThreatMatchValue
* Updating types and tests
  * Adds to the previously empty ThreatEcs

* Revert "Pass full fields data to our row renderers"

This reverts commit 19c93ee.

We ended up extending the existing data (albeit from the fields
response!).

* Fix draggables

* adds contextId and eventId to pass to draggable
* We don't have a order-independent key for each individual
  ThreatMatchRow, due to matched.id not being mapped/returned in the
  fields response
* Fixes up a few things related to using the new data format

* Move indicator field strings to constants

* Fix example data for CTI row renderer

* Adds missing Threat ECS types

* Move CTI field constants to common folder

In order to use these in both the row renderer and the server request,
we need to move them to common/

* Remove redundant CTI fields from client request

These are currently hardcoded on the backend of the events/all query
(via TIMELINE_EVENTS_FIELDS); declaring them on both ends is arguably
confusing, and we're going with YAGNI for now.

* Add missing graphQL type

This was causing type errors as this enum exists both here and in
common/, and I had only updated one of them.

* Updates tests

One is still failing due to an outdated test subject, but I expect this
to change after an upcoming meeting so leaving it for now.

* Split ThreatMatchRow into subcomponents

One for displaying match details, and another for indicator details

The indicator details will be sparse, so there's going to be some
conditional rendering in there.

* Make CTI row renderer look nice

* Adds translations for copy
* Fixes most of our layout woes with more flexbox!
* Conditional rendering of indicator details based on data
* tests

* Make indicator reference field an external link

Leverages the existing FormattedFieldValue component, with one minor
tweak to add this field to the URL allowlist.

* Back to consistent horizontal spacing, here

The draggable badges are a little odd in that their full box isn't
indicated until hover, making the visual weight a little off.

* Add hr as a visual separator between each match "row" of the row renderer

* Fix tests broken due to addition of a new row renderer

These tests are all implicitly testing the list of row renderers.

* Full-width hr

At certain container widths, a half-width hr is not sufficient.

* More descriptive constant

Obviates the need for the accompanying comments.

* More realistic data

Also ensures less traffic to urlhaus ;)

* Remove useless comment

* Add threat_match row renderer type to GQL client

Gennin' beanz

* Ensure contextId is unique for each CTI subrow

We need to add the row index to our contextId to ensure that our
draggables work correctly for multiple rows, since each row will
necessarily have the same eventId and timelineId.

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 16, 2021
…#97347)

* Move alert-specific mocks to more declarative mock file

* Add placeholder interface for ECS threat fields

* Test and implement CTI row renderer

The display details are not yet implemented, but those will be fleshed
out in the ThreatMatchRow component.

* Pass full fields data to our row renderers

This data is not used by any existing row renderers and so this commit
is mostly just plumbing that data through.

This is necessary, however, for our new threat match row renderer as it
requires nested fields, which cannot be retrieved through the mechanism
that retrieves the existing row renderer data. However, these nested
fields are available, if requested, through this other data structure,
hence this plumbing.

For now to minimize changes I'm marking this as an optional field;
however in reality a value will always be present.

* Rewrite existing row renderer in terms of flattened data

Updates logic, tests and mocks accordingly.

* Moving logic into discrete files

* helpers
* explicit fields file, which will hopefully be part of the renderer API
  at some point
* parent component to split data into "rows" as defined by our renderer
* row component for stateless presentation of a single match

* Register threat match row rendere

Adds tentative copy, example row, and accompanying mock data.

* WIP: Rendering draggable fields but hit the data loss issue with nested fields being flattened

* WIP: implementing row renderer against new data format

I haven't yet deleted the old (new?) unused path yet. Cleanup to come.

* Updating based on new data

* Rewrites isInstance logic for new data as helper, hasThreatMatchValue
* Updating types and tests
  * Adds to the previously empty ThreatEcs

* Revert "Pass full fields data to our row renderers"

This reverts commit 19c93ee.

We ended up extending the existing data (albeit from the fields
response!).

* Fix draggables

* adds contextId and eventId to pass to draggable
* We don't have a order-independent key for each individual
  ThreatMatchRow, due to matched.id not being mapped/returned in the
  fields response
* Fixes up a few things related to using the new data format

* Move indicator field strings to constants

* Fix example data for CTI row renderer

* Adds missing Threat ECS types

* Move CTI field constants to common folder

In order to use these in both the row renderer and the server request,
we need to move them to common/

* Remove redundant CTI fields from client request

These are currently hardcoded on the backend of the events/all query
(via TIMELINE_EVENTS_FIELDS); declaring them on both ends is arguably
confusing, and we're going with YAGNI for now.

* Add missing graphQL type

This was causing type errors as this enum exists both here and in
common/, and I had only updated one of them.

* Updates tests

One is still failing due to an outdated test subject, but I expect this
to change after an upcoming meeting so leaving it for now.

* Split ThreatMatchRow into subcomponents

One for displaying match details, and another for indicator details

The indicator details will be sparse, so there's going to be some
conditional rendering in there.

* Make CTI row renderer look nice

* Adds translations for copy
* Fixes most of our layout woes with more flexbox!
* Conditional rendering of indicator details based on data
* tests

* Make indicator reference field an external link

Leverages the existing FormattedFieldValue component, with one minor
tweak to add this field to the URL allowlist.

* Back to consistent horizontal spacing, here

The draggable badges are a little odd in that their full box isn't
indicated until hover, making the visual weight a little off.

* Add hr as a visual separator between each match "row" of the row renderer

* Fix tests broken due to addition of a new row renderer

These tests are all implicitly testing the list of row renderers.

* Full-width hr

At certain container widths, a half-width hr is not sufficient.

* More descriptive constant

Obviates the need for the accompanying comments.

* More realistic data

Also ensures less traffic to urlhaus ;)

* Remove useless comment

* Add threat_match row renderer type to GQL client

Gennin' beanz

* Ensure contextId is unique for each CTI subrow

We need to add the row index to our contextId to ensure that our
draggables work correctly for multiple rows, since each row will
necessarily have the same eventId and timelineId.

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Ryland Herrick <[email protected]>
madirey pushed a commit to madirey/kibana that referenced this pull request May 11, 2021
…#96275)

* Move alert-specific mocks to more declarative mock file

* Add placeholder interface for ECS threat fields

* Test and implement CTI row renderer

The display details are not yet implemented, but those will be fleshed
out in the ThreatMatchRow component.

* Pass full fields data to our row renderers

This data is not used by any existing row renderers and so this commit
is mostly just plumbing that data through.

This is necessary, however, for our new threat match row renderer as it
requires nested fields, which cannot be retrieved through the mechanism
that retrieves the existing row renderer data. However, these nested
fields are available, if requested, through this other data structure,
hence this plumbing.

For now to minimize changes I'm marking this as an optional field;
however in reality a value will always be present.

* Rewrite existing row renderer in terms of flattened data

Updates logic, tests and mocks accordingly.

* Moving logic into discrete files

* helpers
* explicit fields file, which will hopefully be part of the renderer API
  at some point
* parent component to split data into "rows" as defined by our renderer
* row component for stateless presentation of a single match

* Register threat match row rendere

Adds tentative copy, example row, and accompanying mock data.

* WIP: Rendering draggable fields but hit the data loss issue with nested fields being flattened

* WIP: implementing row renderer against new data format

I haven't yet deleted the old (new?) unused path yet. Cleanup to come.

* Updating based on new data

* Rewrites isInstance logic for new data as helper, hasThreatMatchValue
* Updating types and tests
  * Adds to the previously empty ThreatEcs

* Revert "Pass full fields data to our row renderers"

This reverts commit 19c93ee.

We ended up extending the existing data (albeit from the fields
response!).

* Fix draggables

* adds contextId and eventId to pass to draggable
* We don't have a order-independent key for each individual
  ThreatMatchRow, due to matched.id not being mapped/returned in the
  fields response
* Fixes up a few things related to using the new data format

* Move indicator field strings to constants

* Fix example data for CTI row renderer

* Adds missing Threat ECS types

* Move CTI field constants to common folder

In order to use these in both the row renderer and the server request,
we need to move them to common/

* Remove redundant CTI fields from client request

These are currently hardcoded on the backend of the events/all query
(via TIMELINE_EVENTS_FIELDS); declaring them on both ends is arguably
confusing, and we're going with YAGNI for now.

* Add missing graphQL type

This was causing type errors as this enum exists both here and in
common/, and I had only updated one of them.

* Updates tests

One is still failing due to an outdated test subject, but I expect this
to change after an upcoming meeting so leaving it for now.

* Split ThreatMatchRow into subcomponents

One for displaying match details, and another for indicator details

The indicator details will be sparse, so there's going to be some
conditional rendering in there.

* Make CTI row renderer look nice

* Adds translations for copy
* Fixes most of our layout woes with more flexbox!
* Conditional rendering of indicator details based on data
* tests

* Make indicator reference field an external link

Leverages the existing FormattedFieldValue component, with one minor
tweak to add this field to the URL allowlist.

* Back to consistent horizontal spacing, here

The draggable badges are a little odd in that their full box isn't
indicated until hover, making the visual weight a little off.

* Add hr as a visual separator between each match "row" of the row renderer

* Fix tests broken due to addition of a new row renderer

These tests are all implicitly testing the list of row renderers.

* Full-width hr

At certain container widths, a half-width hr is not sufficient.

* More descriptive constant

Obviates the need for the accompanying comments.

* More realistic data

Also ensures less traffic to urlhaus ;)

* Remove useless comment

* Add threat_match row renderer type to GQL client

Gennin' beanz

* Ensure contextId is unique for each CTI subrow

We need to add the row index to our contextId to ensure that our
draggables work correctly for multiple rows, since each row will
necessarily have the same eventId and timelineId.

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team: CTI Team:Threat Hunting Security Solution Threat Hunting Team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants